-
Notifications
You must be signed in to change notification settings - Fork 2
[#142] Consolidate discovery handlers into unified class #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Consolidates entity discovery and data endpoints by replacing per-entity handler classes with unified DiscoveryHandlers and DataHandlers, and adds cache-level data aggregation to support entity-agnostic /data operations across areas/components/apps/functions.
Changes:
- Replaced per-entity discovery handlers with a unified
DiscoveryHandlers. - Introduced unified
DataHandlersplus newThreadSafeEntityCachedata aggregation APIs/structs to support/dataacross entity types. - Updated REST routing and build configuration; removed legacy
handlers/discovery/*handler files.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp | Adds data aggregation implementation used by unified data endpoints. |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Rewires routes to unified handlers; adds new area/function data routes. |
| src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp | New consolidated discovery endpoint implementations. |
| src/ros2_medkit_gateway/src/http/handlers/discovery/function_handlers.cpp | Removed legacy function discovery handler implementation. |
| src/ros2_medkit_gateway/src/http/handlers/discovery/component_handlers.cpp | Removed legacy component discovery/data handler implementation. |
| src/ros2_medkit_gateway/src/http/handlers/discovery/area_handlers.cpp | Removed legacy area discovery handler implementation. |
| src/ros2_medkit_gateway/src/http/handlers/discovery/app_handlers.cpp | Removed legacy app discovery/data handler implementation. |
| src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp | New consolidated /data endpoint implementations across entity types. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/models/thread_safe_entity_cache.hpp | Adds AggregatedData/TopicData and new data aggregation APIs. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp | Updates handler member fields to unified handler classes. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp | Updates umbrella handler includes to unified discovery/data handlers. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery_handlers.hpp | New public header for consolidated discovery handlers. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/function_handlers.hpp | Removed legacy function handler header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/component_handlers.hpp | Removed legacy component handler header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/area_handlers.hpp | Removed legacy area handler header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/app_handlers.hpp | Removed legacy app handler header. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/data_handlers.hpp | New public header for consolidated data handlers. |
| src/ros2_medkit_gateway/CMakeLists.txt | Removes legacy discovery handler sources; adds new unified handler sources. |
src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp
Outdated
Show resolved
Hide resolved
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery_handlers.hpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp
Outdated
Show resolved
Hide resolved
Changes:
- Fix get_area_data() to recursively traverse subareas using area_to_subareas_
for proper area hierarchy aggregation behavior
- Fix handle_list_data() to use topic.name as ID instead of normalized form,
making IDs directly usable for GET/PUT calls (clients URL-encode as needed)
- Fix _ensure_app_data_ready() helper to poll /apps/{id}/data endpoint
instead of /apps/{id} to match what tests actually depend on
- Fix unused topic_names variable in test_111 by adding it to print output
- Fix function discovery: call discover_functions() in refresh_cache() and
pass results to thread_safe_cache_.update_all() (was passing empty {})
- Apply clang-format to modified files
This fixes issues with:
- Area data not aggregating topics from subareas
- Data list IDs not being usable for subsequent GET/PUT item requests
- Test flakiness due to polling wrong endpoint
- Function data/operations endpoints returning 404 (functions not in cache)
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery_handlers.hpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
- Create unified DiscoveryHandlers class with entity-agnostic methods - Create unified DataHandlers class for data operations across entities - Add AggregatedData and TopicData structs to ThreadSafeEntityCache - Remove handlers/discovery/ folder (8 files, ~1600 lines) - Update rest_server to use discovery_handlers_ and data_handlers_ - ~55% code reduction through handler consolidation Discovery endpoints now handled by single class: - Areas: list, get, components, subareas, contains - Components: list, get, subcomponents, hosts, depends-on - Apps: list, get, depends-on - Functions: list, get, hosts Data endpoints unified across all entity types via DataHandlers.
- Add missing #include <algorithm> for std::count in data_handlers.cpp - Fix handle_list_data() to check entity existence before source_ids - Fix handle_put_data_item() to mirror GET logic for full_topic_path - Change discovery_handlers.hpp to use #pragma once - Add Cap::FAULTS to handle_get_app() capabilities list - Add entity_id to source_ids in get_area_data/get_function_data - Fix direction='both' detection in collect_topics_from_app/component These changes ensure: - Consistent include guard style across codebase - Correct 200/404 for empty vs missing entities - No double slash in topic paths for PUT - Accurate pub/sub direction metadata in aggregated responses
…andlers
Add integration tests for aggregated data endpoints:
- test_109: GET /areas/{area_id}/data for valid area
- test_110: GET /areas/{area_id}/data for nonexistent area
- test_111: GET /areas/root/data for system-wide aggregation
- test_112: GET /areas/{area_id}/data returns 200 for empty areas
- test_113: GET /functions/{function_id}/data for valid function
- test_114: GET /functions/{function_id}/data for nonexistent function
- test_115: GET /functions/{function_id}/data rejects invalid IDs
Fix existing tests for unified handler error messages:
- Update tests to expect 'Entity not found' / 'Invalid entity ID'
- Update tests to use 'entity_id' parameter in error responses
- Add readiness helpers to tests 07-10 for discovery race condition
- Allow direction='both' in topic data assertions
- Use 'aggregated_from' field name in new tests
Apply clang-format to handler files.
Changes:
- Fix get_area_data() to recursively traverse subareas using area_to_subareas_
for proper area hierarchy aggregation behavior
- Fix handle_list_data() to use topic.name as ID instead of normalized form,
making IDs directly usable for GET/PUT calls (clients URL-encode as needed)
- Fix _ensure_app_data_ready() helper to poll /apps/{id}/data endpoint
instead of /apps/{id} to match what tests actually depend on
- Fix unused topic_names variable in test_111 by adding it to print output
- Fix function discovery: call discover_functions() in refresh_cache() and
pass results to thread_safe_cache_.update_all() (was passing empty {})
- Apply clang-format to modified files
This fixes issues with:
- Area data not aggregating topics from subareas
- Data list IDs not being usable for subsequent GET/PUT item requests
- Test flakiness due to polling wrong endpoint
- Function data/operations endpoints returning 404 (functions not in cache)
8e76a1c to
670883e
Compare
Discovery handlers: - handle_function_hosts: move is_online to x-medkit, add _links, total_count in x-medkit - handle_list_apps/functions: use ThreadSafeEntityCache for consistency with data endpoints - discovery_handlers.hpp: add missing httplib.h include Data handlers: - Fix ID consistency: use full_topic_path directly in GET/PUT responses - Remove unused normalize_topic_to_id/id_to_topic functions from http_utils.hpp Gateway node: - Remove unused timestamp variable (fixes -Wunused-but-set-variable) Tests: - Change SkipTest to fail() in _ensure_app_data_ready for deterministic CI
- Add topic_type_cache_ member for O(1) topic type lookup - Add update_topic_types() writer method and get_topic_type() reader method - Populate topic types during gateway refresh_cache() - Use cached types in handle_list_data() instead of discover_all_topics() This avoids expensive ROS graph queries on every /data request, which could add up on systems with hundreds of topics. Addresses review comment from mfaferek93.
Add DataAggregationTest fixture to test_entity_resource_model.cpp with tests covering: - AppDataReturnsOwnTopicsOnly: verify app returns only its own topics - ComponentDataAggregatesFromHostedApps: verify component aggregates topics from itself and hosted apps - DirectionMergesToBothWhenPubAndSub: verify direction merging when same topic is published and subscribed - AreaDataAggregatesFromAllComponents: verify area aggregates from all components in hierarchy - GetEntityDataAutoDetectsType: verify auto-detection of entity type - UnknownEntityReturnsEmptyData: verify empty result for nonexistent entity
Changes:
- Fix get_area_data() to recursively traverse subareas using area_to_subareas_
for proper area hierarchy aggregation behavior
- Fix handle_list_data() to use topic.name as ID instead of normalized form,
making IDs directly usable for GET/PUT calls (clients URL-encode as needed)
- Fix _ensure_app_data_ready() helper to poll /apps/{id}/data endpoint
instead of /apps/{id} to match what tests actually depend on
- Fix unused topic_names variable in test_111 by adding it to print output
- Fix function discovery: call discover_functions() in refresh_cache() and
pass results to thread_safe_cache_.update_all() (was passing empty {})
- Apply clang-format to modified files
This fixes issues with:
- Area data not aggregating topics from subareas
- Data list IDs not being usable for subsequent GET/PUT item requests
- Test flakiness due to polling wrong endpoint
- Function data/operations endpoints returning 404 (functions not in cache)
Discovery handlers: - handle_function_hosts: move is_online to x-medkit, add _links, total_count in x-medkit - handle_list_apps/functions: use ThreadSafeEntityCache for consistency with data endpoints - discovery_handlers.hpp: add missing httplib.h include Data handlers: - Fix ID consistency: use full_topic_path directly in GET/PUT responses - Remove unused normalize_topic_to_id/id_to_topic functions from http_utils.hpp Gateway node: - Remove unused timestamp variable (fixes -Wunused-but-set-variable) Tests: - Change SkipTest to fail() in _ensure_app_data_ready for deterministic CI
Pull Request
Summary
Discovery endpoints now handled by single class:
Issue
Link the related issue (required):
Type
Testing
How was this tested / how should reviewers verify it?
Checklist